This is getting very very complicated.

We have 'deploy', 'execute' and now _do_execute and post_execute.

This class was supposed to be a fairly clean interface between agent and handler. I was reluctant against adding an implementation to the deploy method at all (#6246 (comment)) but I thinks this takes it too far.

I think, with this structure, there is so much inversion of control, that to know what each sub class does, you have to read up-and-down the inheritance hierarchy and piece it back together. These methods have no clean function of their own, only in relation to the implementation of other methods.

I see why you lifted out this error handling code, but I wonder if there is a better way.

For post-execute, I think the easiest way of removing this is to have the subclass override execute and do

def execute(...):
   super().execute(..._
   if _should_reload():
                self.do_reload(ctx, resource)

For the exception handling, I wonder if we can lift it up/duplicate it higher up in the call stack, that would both make it more robust and more out-of-sight?

I mostly agree with this. A few additional notes:

1. I think we could consider keeping _do_execute iff we more clearly distinguish the implementor interface from the user interface, keeping the footprint for the handler developer small and intuitive.
2. Even though I think we can get away with it, I don't necessairly think we should. Is there a reason we can't lift the error handling and the call to pre and post into deploy directly? This may be what Wouter is proposing as well, I'm not sure.
3. For the post execute I wholeheartedly agree.

Ah, 2 and 3 are not compatible...

I think we could consider keeping _do_execute iff we more clearly distinguish the implementor interface.

I tried to make it clear with the underscore that this was not part of the public API.

Is there a reason we can't lift the error handling and the call to pre and post into deploy directly? This may be what Wouter is proposing as well, I'm not sure.

Wouter was proposing to move it out of the hander. That means moving it into the ResourceAction for example.

The main question is: Do you feel the same way as Wouter? Namely that this implementation results in very complicates and hard to understand code. And if so, do you see a good way to keep the benefits while making it easier to understand? If we cannot come to a better implementation, I would just roll it back and then we just trade better readability for code duplication.

that this implementation results in very complicates and hard to understand code

Code not so much, but I do find that it "pollutes" the interface. Because, while you say it's not part of the public API, handlers are still supposed to implement it. On the other hand I'm inclined to agree that the error handling code belongs in the default implementation because it is part of the generic concept of a resource + handler.

Wouter was proposing to move it out of the hander.

Ok, I misinterpreted that. But my proposal still stands, why not move it to deploy? Afaik that shouldn't break any of our current handlers, and I feel like it would fit with the currently defined contracts of both deploy and execute. Perhaps we have to add one more sentence to the deploy docstring to be explicit but the purpose of deploy is essentially to inspect dependencies' state, trigger the actual deploy (by calling into execute) and react appropriately to any unexpected state. I don't think this would break any of our existing handlers, would it?

I'm leaving post_execute out of it for now because I feel like _do_execute is the big one and the other discussion hinges slightly on which way we go here.

I am fine with moving the error handling logic into deploy(). However, this would mean we also have to move the pre() and post() methods to deploy(). So the behavior will change when someone overrides the execute() method directly. I can't find a module that does that. If we don't have one, I think we can safely move the error handling logic into deploy().

Afaik we don't have one, and this is iso7 so I think we have some freedom here as long as we mention it in a change entry. From where I'm standing, I like this idea best among our options. What about you?

I can't find one either.

Wow, I had no clue it was required to list all type vars here. I really can't wait for the 3.12 syntax.